✨(CLDSRV-813) update CloudServer XML ListObjectsV2 to support optional attributes #6043
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## feature/CLDSRV-812/list-objects-v2-optional-permissions #6043 +/- ##
===========================================================================================
- Coverage 84.36% 84.33% -0.03%
===========================================================================================
Files 204 204
Lines 12935 12949 +14
===========================================================================================
+ Hits 10912 10920 +8
- Misses 2023 2029 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
DarkIsDude
force-pushed
the
feature/CLDSRV-813/optional-attributes-response
branch
2 times, most recently
from
229fe35 to
1fde211
Compare
DarkIsDude
force-pushed
the
feature/CLDSRV-813/optional-attributes-response
branch
from
1fde211 to
a8eba02
Compare
| for (const key of Object.keys(item)) { | ||
| if (key.startsWith('x-amz-meta-')) { | ||
| if (optionalAttributes.includes('x-amz-meta-*') || optionalAttributes.includes(key)) { | ||
| xml.push(`<${key}>${item[key]}</${key}>`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
it would be more efficient to go the other way round:
- e.g. go through every
optionalAttributes(which are more likely not as numerous as the fields in our metadata objects) - in particular, this also ensure we keep the "usual" case (e.g. without optionalAttributes) optimized.
- it avoids the repeated lookups in
optionalAttributes
for(const attr in optionalAttributes) {
switch (attr) {
case 'RestoreStatus':
[...]
break;
case 'x-amz-meta-*':
for ((key, value) in item.getUserMetadata()) {
xml.push(`<${key}>${item[key]}</${key}>`);
}
break;
default:
const v = item[attr];
if (v !== undefined) {
xml.push(....)
}
break;
}
The x-amz-meta-* case is a bit more concerning, since
- it needs to go through every field to get the every user metadata (but there is already the
ObjectMD.getUserMetadata()function to do this) - there could be an issue if both
x-amz-meta-*and specific fields are specified ; but this should be rejected when validating the request?
This approach is also more future-proof, as it will allow to ad more fields by simply adding them to the optionalAttributes list during validation (i.e. less coupling, only need to update one place if we add more fields later)
| } | ||
|
|
||
| function clearUploadIdFromVersions(versions) { | ||
| function clearUploadIdFromVersionsAndRestoreStatus(versions) { |
There was a problem hiding this comment.
| function clearUploadIdFromVersionsAndRestoreStatus(versions) { | |
| function clearUploadIdAndRestoreStatusFromVersions(versions) { |
| for (const version of versions) { | ||
| if (version.value) { | ||
| version.value.uploadId = undefined; | ||
| version.value.restoreStatus = undefined; |
There was a problem hiding this comment.
why do we need to clear RestoreStatus from all of these tests?
RestoreStatus is only set by cold storage, so will not be set in most case : so seems best to keep this "as is" and handle the cold-storage tests
|
|
||
| for (const version of versions) { | ||
| if (version.value) { | ||
| version.value.restoreStatus = undefined; |
There was a problem hiding this comment.
why do we need to clear RestoreStatus in every test?
all APIs should behave the same as before, so you should not need to tweak the tests
(and resetting restoreStatus may be expected -if anywhere- only in tests where you do actually invoke the list object v2 api with optional attributes)
| done(); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
the x-amz-optional-attributes header tests from previous PR should be updated: some tests where actually not really testing much (or duplicated) since the result was not observable...
| assert.strictEqual(err, null); | ||
| assert.strictEqual(result.ListBucketResult.$.xmlns, 'http://s3.amazonaws.com/doc/2006-03-01/'); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
would be nice to have some functional tests as well : i.e. tests using the AWS sdk to retrieve the values from a running cloudserver service.
As an extra benefit, it would also provide an exemple of how to use the API.
There was a problem hiding this comment.
Add a new test on lowerCase (all usermetadata are lowercase)
Related issues
https://scality.atlassian.net/browse/CLDSRV-813